fix(websocket): don't panic on a plain HTTP request to a WebSocket route - #3932
fix(websocket): don't panic on a plain HTTP request to a WebSocket route#3932om7057 wants to merge 4 commits into
Conversation
|
@Umang01-hash @coolwednesday a small reminder on this! |
aryanmehrotra
left a comment
aryanmehrotra
left a comment
There was a problem hiding this comment.
Sorry for the wait, and for missing your ping — thirteen days on a remotely-triggerable panic is too
long.
Bug is real, fix is right. Ran both builds in containers:
development 500 Internal Server Error + panic: interface conversion: interface {} is nil, not string
this branch 500 couldn't establish connection to web socket + no panics
Both guards are needed — GetWebsocketConnection does return a nil *Connection for an unknown ID.
One change: the status code on the first return site.
The issue asked for 4xx and this keeps the pre-existing 500. Not a regression, but RFC 6455 §4.2.1
says a failed handshake gets "an appropriate error code (such as 400 Bad Request)" — and we already
do that twelve lines away, in middleware/web_socket.go:21, when an upgrade is attempted and fails.
A request that never attempted one should not be treated as more serious than one that tried.
Only the first site though — the two are different failures:
if !ok {
return nil, websocket.ErrorConnection // plain GET, no upgrade -> client error, 400
}
if conn == nil || conn.Conn == nil {
return nil, websocket.ErrorConnection // upgrade ran, connection missing -> server fault, keep 500
}So please return a distinct error at the first site rather than putting StatusCode() on the shared
ErrorConnection — folding both into a 4xx would hide a genuine bug class. ErrorConnection has no
other references in the repo and only your new test pins a status, so it is a one-line assertion
change.
(426 looks tempting but is wrong here — RFC 6455 reserves it for version negotiation in §4.2.2.)
Also: this branch predates #4051 so it does not build standalone against current development.
Merges cleanly — I tested the merged result — just worth a rebase.
Addresses gofr-dev#3862. App.WebSocket registered its handler with an unsafe type assertion on WSConnectionKey: a plain GET with no Upgrade headers never gets that key set on the request context by WSHandlerUpgrade, so the assertion panicked with "interface conversion: interface {} is nil, not string". GetWebsocketConnection also returns a nil *Connection for an unknown ID, so the subsequent conn.Conn == nil check was itself a nil pointer dereference once reached. Both call sites now check ok/nil before dereferencing and return the existing websocket.ErrorConnection instead of panicking.
Use http.NewRequestWithContext + http.DefaultClient.Do instead of the context-less http.Get.
…cket upgrade Addresses review feedback on gofr-dev#3932: the "no WSConnectionKey on context" case (a plain HTTP request that never went through the upgrade handshake) is a client error, distinct from the "upgrade ran but the connection is missing" case, which is a server-side fault. Splitting them keeps the existing 500 for the latter and adds a new websocket.ErrorNotWebSocketUpgrade type for the former, mapping to 400 per RFC 6455 4.2.1, consistent with the 400 middleware/web_socket.go already returns when an upgrade attempt fails.
09bd23a to
ce48537
Compare
Sure, no worries @aryanmehrotra. Also rebased onto current development, builds standalone now. |
2 participants
Summary
Fixes #3862.
App.WebSocketregistered its handler with an unsafe type assertion onWSConnectionKey. A plain HTTP GET to a WebSocket route (noConnection: Upgrade/Upgrade: websocketheaders) never gets that key set on the request context by the upgrade middleware, so the assertionctx.Request.Context().Value(websocket.WSConnectionKey).(string)panicked withinterface conversion: interface {} is nil, not string.GetWebsocketConnectionalso returns a nil*Connectionfor an unknown connection ID, so even a safe type assertion would still hit a nil pointer dereference at the followingconn.Conn == nilcheck.Both call sites now check
ok/nilbefore dereferencing and return the existingwebsocket.ErrorConnectioninstead of panicking, matching the fix direction suggested in the issue.Test plan
go build ./...go vet ./...go test ./pkg/gofr/...Test_WebSocket_PlainHTTPRequestDoesNotPanicsends a plainGETto a route registered viaapp.WebSocketand asserts the response carrieswebsocket.ErrorConnection's message rather than the generic panic-recovery message (both map to HTTP 500, so the response body is the only way to distinguish "handled cleanly" from "panicked and got recovered"). Verified this test fails with the exact panic from the issue when the fix is reverted, and passes with it applied.